Skip to content

ssh.c: don't fail a channel read whose window credit is deferred - #1192

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/channel_stream_read
Open

ssh.c: don't fail a channel read whose window credit is deferred#1192
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/channel_stream_read

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

src/ssh.c has three channel read paths. _ChannelReadExt() already separates
"bytes were delivered" from "the window-adjust packet made it onto the wire".
The other two conflate them.

_ChannelRead() — behind wolfSSH_ChannelIdRead() and wolfSSH_ChannelRead()
consumes the bytes, then returns the send result:

inputBuffer->idx += bufSz;                    /* bytes consumed */
updateResult = _UpdateChannelWindow(channel);
if (updateResult == WS_SUCCESS)
    updateResult = bufSz;
return updateResult;                          /* WS_WANT_WRITE -> "failed" */

_UpdateChannelWindow() returns WS_WANT_WRITE whenever the adjust would block —
routine on a non-blocking socket, not an error. The caller is told the read failed
after the data has already left the input buffer, so those bytes are gone, and a
caller using the usual if (cnt_r <= 0) break; shape tears the connection down
(src/wolfscp.c:1762 is one). wolfSSH_stream_read() has the same shape without the
loss — it skips inputBuffer->idx += n, so the data is re-delivered — but still
reports a failure for a read that copied n bytes.

SendPacket() sets ssh->error only for WS_WANT_WRITE, so a hard transport failure
during the adjust was invisible on both paths.

Fix (src/ssh.c)

Both paths now report the bytes copied and keep the send result out of band, matching
_ChannelReadExt():

  • _ChannelRead() returns bufSz unconditionally; wolfSSH_stream_read()
    advances inputBuffer->idx before the adjust, then returns n.
  • Both record a non-success adjust result in ssh->error and WLOG at
    WS_LOG_ERROR for anything other than WS_WANT_WRITE.
  • _ChannelRead() also clears a stale WS_WANT_WRITE once the adjust does go out
    with the output buffer drained — its entry points, unlike wolfSSH_stream_read(),
    do not reset ssh->error.

A deferred credit still goes out: wolfSSH_worker() and the next SendChannelData()
flush pending output before doing anything else.

API note: wolfSSH_stream_read(), wolfSSH_ChannelRead() and
wolfSSH_ChannelIdRead() now return the byte count where a deferred or failed window
adjust previously produced a negative return. Callers that relied on that negative
return must check wolfSSH_get_error() after a successful read. Documented in
wolfssh/ssh.h and the wolfSSH_stream_read() block comment.

Tests (tests/unit.c)

One harness per path, each putting a full window of channel data and reading it back:

Phase IO send Asserts
Deferred credit WS_CBIO_ERR_WANT_WRITE byte count, payload, ssh->error, credited window, consumed buffer
Hard failure WS_CBIO_ERR_GENERAL byte count still returned, ssh->error == WS_SOCKET_ERROR_E, credit left owed
Clean credit (wolfSSH_ChannelIdRead() only) full send ssh->error back to WS_SUCCESS, no credit owed

Verification

  • unit.test, api.test, testsuite.test and scripts/{sftp,scp,fwd,get-put}.test
    all pass; unit.test also clean under ASan + UBSan.
  • Clean under -Werror with gcc-13 across 6 configurations (enable-all, Zephyr
    defines, sftp-only, scp-only, default, small-stack).
  • Negative controls: reverting the ssh->error recording fails both hard-failure
    assertions; reverting the stale-WS_WANT_WRITE clearing fails the third phase.

The echo server's worker loop has its own partial-write problems in this area. Those
are a separate rework PR that builds on this one — deliberately not in scope here.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 24, 2026
Copilot AI lite review requested due to automatic review settings August 24, 2026 07:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes a correctness issue in wolfSSH’s channel read paths where a deferred CHANNEL_WINDOW_ADJUST send (e.g., WS_WANT_WRITE on non-blocking sockets) could cause the read API to report failure even after bytes were already delivered/consumed, leading callers to prematurely tear down connections.

Changes:

  • Update wolfSSH_stream_read() and the internal _ChannelRead() path (used by wolfSSH_ChannelIdRead() / wolfSSH_ChannelRead()) to always return the number of bytes copied/consumed, decoupling that from the window-adjust send result.
  • Record non-success window-adjust send results in ssh->error (and log hard failures), matching the “bytes delivered vs. credit flushed” split already used by _ChannelReadExt().
  • Add targeted unit tests covering both affected read entry points under a WS_WANT_WRITE send scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/ssh.c Ensures channel/stdout read APIs report bytes read even when window-adjust send is deferred, while still surfacing the deferred/failed credit via ssh->error.
tests/unit.c Adds unit tests validating correct byte reporting, payload integrity, local window crediting, and ssh->error behavior under deferred window-adjust sends.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1192

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread tests/unit.c
Comment thread tests/unit.c
- wolfSSH_stream_read() advances inputBuffer->idx before
  _UpdateChannelWindow() and returns the byte count, recording a
  non-success adjust result in ssh->error and logging anything
  other than WS_WANT_WRITE.
- _ChannelRead() returns the bytes copied, records the adjust
  result the same way, and clears a stale WS_WANT_WRITE when the
  adjust goes out with the output buffer drained.
- The wolfSSH_stream_read() block comment and a new note above its
  wolfssh/ssh.h declaration state that wolfSSH_get_error() carries
  the window-adjust status on a successful read.
- tests/unit.c adds test_stream_read_deferredWindowAdjust() and
  test_ChannelIdRead_deferredWindowAdjust(), each reading a full
  window through an IO send that reports WS_CBIO_ERR_WANT_WRITE
  and then through one that fails, checking the byte count, the
  payload, ssh->error, the window credit and the drained input
  buffer.
- The wolfSSH_ChannelIdRead() test ends on a credit that sends
  cleanly, asserting ssh->error returns to WS_SUCCESS and no
  credit stays owed.
@yosuke-wolfssl
yosuke-wolfssl force-pushed the fix/channel_stream_read branch from 8352ae7 to ca90ef2 Compare August 24, 2026 23:44
Comment thread tests/unit.c
Comment thread tests/unit.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1192

Scan targets checked: wolfssh-bugs, wolfssh-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

@wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot dismissed their stale review August 25, 2026 00:08

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants